would a condition like this do what we want?

if parse.peek_keywords(BEGIN, TRANSACTION) {
    None
} else if parse_keyword(BEGIN) {
    Some(parser.parse_begin_exception_end())
}

its not super clear to me why the current logic looks at WORK, TRY etc keywords?

Thanks for the review. These keywords are checked because parse_begin() handles more than just BEGIN TRANSACTION. It also handles WORK, TRY, CATCH, DEFERRED, IMMEDIATE, EXCLUSIVE (since MsSql's supports_start_transaction_modifier() returns true), as well as bare BEGIN;. If any of these are missed, they'd be incorrectly routed to parse_begin_exception_end() and fail to parse.

For example, BEGIN TRY ... END TRY is valid MSSQL syntax. If we only check for BEGIN TRANSACTION, then BEGIN TRY would fall through to parse_begin_exception_end(), which would try to parse TRY as a SQL statement and fail. because TRY is supposed to be handled as a

Feel free to let me know if there are any not clear, thanks!

Oh I see, that makes sense! Can we do something like this to clarify the code?

we pull out the linked fallback logic into a function parse_transaction_modifier(). So that it can be reused.

Then here we can do e.g.

if without_modifier = self.maybe_parse(|parser| {
    if parser.parse_transaction_modifier()?.is_some() {
        parser_error!()
    } else {
        Some(())
    }
})?.is_some();

such that with that bool we know whether to defer to the main parser logic or continue with the begin/exception logic

Thanks for the really nice suggestion. I just updated it does more clean and work as well.